-
Notifications
You must be signed in to change notification settings - Fork 29
Separate harness implementation from the harness framework #446
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Could you fix this warning? (I'm sorry if it wasn't new)
Do you know why the stackprof harness is loaded on the |
eccdfb4 to
d3153ab
Compare
README.md
Outdated
|
|
||
| ``` | ||
| ruby benchmarks/some_benchmark.rb | ||
| ./run_once.rb benchmarks/some_benchmark.rb |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Being able to run a benchmark with just ruby benchmark.rb is a fundamental interface to this project. The low distance between ruby and benchmark code makes it easy to run and tweak for JIT work. I'm against sacrificing this for code organization reasons.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also it makes e.g. JIT bisect easier
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(aside: I was actually looking for this section yesterday but failed to find it, probably I searched for -Iharness or so. I recall this section because I wrote it)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't you still run ruby benchmarks/some_benchmark? You just need to use -r with an path:
ruby -r./harness/perf benchmarks/some_benchmark.rb
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, this isn't only about code organization. The interface to select harness isn't good either. Playing with load paths to be able to select harness isn't usual and it is confusing. I also realize that introducing a script doesn't work, although it doesn't prevent you to use ruby to run the benchmarks. I think of the options, the least worse is using environment variables, in addition to the -r. I'll change it to do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I put back to the documentation ruby benchmarks/some_benchmark.rb as the preferred way to run single benchmarks. I also added support to set the harness using environment variable.
c82ec54 to
82e0b03
Compare
Use file based harness instead of directory based harness
… selection So we don't need to deal with the `-r` ruby argument directly for selecting harnesses.
This allow `ruby` script direct calls to still select the harness without using any wrapper script.
82e0b03 to
01698a8
Compare
|
Given there is a My concern is I think we should not introduce "yet another way to run benchmarks". To be fair |
eregon
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but I would just remove run_once.rb, it seems unnecessary and we already have 2 ways to run benchmarks (run_benchmarks.rb and directly with ruby ... benchmark.rb)
| # Run once with YJIT stats | ||
| ruby --yjit-stats benchmarks/railsbench/benchmark.rb |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't run a single iteration so it's not "once", unlike the ./run_once.sh --yjit-stats benchmarks/railsbench/benchmark.rb before. Trivial to fix: #446 (comment)
| # STACKPROF_OPTS='mode:object' MIN_BENCH_TIME=0 MIN_BENCH_ITRS=1 ruby -v -I harness-stackprof benchmarks/.../benchmark.rb | ||
| # STACKPROF_OPTS='mode:cpu,interval:10' MIN_BENCH_TIME=1 MIN_BENCH_ITRS=10 ruby -v -I harness-stackprof benchmarks/.../benchmark.rb | ||
| # STACKPROF_OPTS='mode:object' HARNESS=stackprof MIN_BENCH_TIME=0 MIN_BENCH_ITRS=1 ruby -v -I harness-stackprof benchmarks/.../benchmark.rb | ||
| # STACKPROF_OPTS='mode:cpu,interval:10' MIN_BENCH_ITRS=10 HARNESS=stackprof MIN_BENCH_TIME=1 MIN_BENCH_ITRS=10 ruby -v -I harness-stackprof benchmarks/.../benchmark.rb |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| # STACKPROF_OPTS='mode:cpu,interval:10' MIN_BENCH_ITRS=10 HARNESS=stackprof MIN_BENCH_TIME=1 MIN_BENCH_ITRS=10 ruby -v -I harness-stackprof benchmarks/.../benchmark.rb | |
| # STACKPROF_OPTS='mode:cpu,interval:10' HARNESS=stackprof MIN_BENCH_TIME=1 MIN_BENCH_ITRS=10 ruby -v -I harness-stackprof benchmarks/.../benchmark.rb |
(MIN_BENCH_ITRS=10 is duplicated)
| ./run_once.sh --yjit-stats benchmarks/railsbench/benchmark.rb | ||
| ```bash | ||
| # Run once with YJIT stats | ||
| ruby --yjit-stats benchmarks/railsbench/benchmark.rb |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| ruby --yjit-stats benchmarks/railsbench/benchmark.rb | |
| HARNESS=once ruby --yjit-stats benchmarks/railsbench/benchmark.rb |
|
I can take a look at the test failures on truffleruby and push a commit to fix them if you'd like, let me know if so. |
I had to spent some time understanding how the harness worked in this project. I realized that there were multiple directories for them and didn't know why.
I later realized that some were different harness implementation, and the harness folder was both the implementation and the framework.
I made this change the separate those two concepts, and remove the need to have different directories for implementation.
The framework lives in
lib/harnessand the implementations live inharness, in separated files (ractor.rb,perf.rb, etc.)